Repository navigation
test: run the --bail JUnit regression tests concurrently and assert the full report - #41423
Conversation
…he full report Both tests in test/regression/issue/26851.test.ts spawn their own bun test child in their own tempDir. Run them with test.concurrent so the two children overlap. Assert the whole JUnit document with an inline snapshot (time and hostname normalized), the bail line and the summary line on stderr, and exit code 1. Add a test after the failure and a third test file to show that --bail stops the run and that the report only holds what ran.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. WalkthroughChangesJUnit bail regression coverage
Merge Risk: ⚪ Minimal · up to This updates regression coverage for JUnit bail output, exit behavior, and skipped tests without changing runtime implementation behavior. No merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: the first CI run showed both children exit 134 on the x64-asan lane. That is the LeakSanitizer abort at the bare bail exit (#32183, fix in #39010), which the old not.toBe(0) assertion hid and which is where the 12s went. The file is now on test/no-validate-leaksan.txt next to 12250.test.ts until #39010 lands. deaf4a7 also strips the CI commit and job env vars from the child, so the snapshot holds when GITHUB_SHA or CI_JOB_URL is set. The PR body has the details and timings. |
… the VM teardown Under the ASAN lane environment, bun test --bail exits with a bare exit(1). The VM teardown is skipped and LeakSanitizer aborts the child with exit code 134 after several seconds (#32183). The old assertions hid this with not.toBe(0). Put the file on the same list as test/regression/issue/12250.test.ts until #39010 lands.
…hild The JUnit reporter adds a <properties> block to each testsuite when GITHUB_SHA, CI_JOB_URL or a similar variable is set. The inline snapshot does not expect it.
There was a problem hiding this comment.
LGTM — the CI env-var stripping in deaf4a7 addresses the <properties> snapshot concern I raised earlier.
What was reviewed: the rewritten runBailWithJUnit helper drains stdout/stderr/exited concurrently and asserts output before exit code; the env-var deletion list matches the keys JunitReporter::generate_properties_list reads; the no-validate-leaksan.txt entry carries a stated reason and tracked issue (#32183) alongside the existing 12250.test.ts entry for the same bail-exit mechanism; the fixture sources are fixed strings so the :2:40/:2:48 column offsets in the snapshot are stable.
Extended reasoning...
Overview
This PR touches two files: it rewrites test/regression/issue/26851.test.ts (a pre-existing regression test for bun test --bail + JUnit reporter) to use test.concurrent, shared helpers, full inline-snapshot assertions on normalized XML, and stricter stderr/exit-code checks; and it adds one entry to test/no-validate-leaksan.txt so the ASAN lane doesn't force BUN_DESTRUCT_VM_ON_EXIT=1 on a child that exits via bare exit(1). No production code is changed.
Security risks
None. This is a test-only refactor of an existing regression test plus a CI-runner allowlist entry. No auth, crypto, network, or user-input parsing is involved.
Level of scrutiny
Low-to-moderate. The one item that would normally warrant scrutiny — opting a test out of LeakSanitizer — is handled per REVIEW.md's requirement that every weakened safety net carry a stated reason: the entry has a two-line comment naming the mechanism (bare exit(1) skipping VM teardown → LSan abort 134) and the tracking issue (#32183), and sits directly beside 12250.test.ts which is on the list for the identical --bail exit path. The PR description names #39010 as the fix after which the entry is removed.
Other factors
I previously flagged that bunEnv spreads process.env without stripping GITHUB_*/CI_* vars, which would make the JUnit reporter emit a <properties> block and break the new inline snapshots on CI or dev boxes with those set. Commit deaf4a7 addresses this by deleting all seven keys the reporter reads before spawning. The rest of the diff follows harness conventions cleanly: tempDir with using, Promise.all on the three pipes, normalizeBunSnapshot for stdout, output assertions before exitCode, test.concurrent for independent subprocess spawns, and .not.toBe(0) tightened to .toBe(1). The added c_never.test.ts fixture and after the failure test give the assertions a negative contract (they must NOT appear), which is a genuine strengthening. No CODEOWNERS entries cover the changed paths.
|
Updated 3:41 AM PT - Sep 5th, 2026
✅ @robobun, your commit deaf4a71bb02cabb71aed16f9070524ce4971f78 passed in 🧪 To try this PR locally: bunx bun-pr 41423That installs a local version of the PR into your bun-41423 --bun |
Problem
test/regression/issue/26851.test.tstakes up to 12s on the debian 13 x64-asan lane. It is in the slowest 5 percent of test files. Both tests spawn abun test --bailchild, and they ran one after the other.bun test --bailexits with a bareexit(1), so theBUN_DESTRUCT_VM_ON_EXITteardown is skipped and LeakSanitizer aborts the child after several seconds of report symbolization (bun test exits after failures are not LSan-clean (bail and failing runs abort under detect_leaks=1) #32183, fix open in bun test: exit through the VM teardown path when --bail stops a run #39010). Locally each child takes 0.3s. Under the lane environment it takes 3.8s. The oldexpect(exitCode).not.toBe(0)hid the abort.Fix
test/no-validate-leaksan.txt, next totest/regression/issue/12250.test.ts, which is on the list for the same bail exit. The entry is to be removed when bun test: exit through the VM teardown path when --bail stops a run #39010 lands. Without LeakSanitizer the children exit 1 in well under a second.test.concurrent. Each test owns its tempDir and its outfile, so the two children overlap. A sharedrunBailWithJUnithelper does the spawn and reads the report.toMatchInlineSnapshot: thetestsuitesroot withtests,assertions,failuresandskipped, onetestsuiteper file that ran, thetestcasenames and lines, and thefailureelement with its type and message. Only thetimeandhostnameattributes are normalized. Assert the child output before the exit code: stdout is the version banner, stderr has the(pass)/(fail)lines, theRan N tests across M files.summary andBailed out after 1 failure. The exit code is exactly 1.c_never.test.ts. Neither appears in stderr or in the report. This proves that--bailstopped the run and that the report holds only what ran. Discovery order of the root directory is sorted by name, soa_passruns beforeb_fail.bun bd test test/regression/issue/26851.test.ts. Plain, 3 runs: before 2.66s to 3.14s wall, after 2.27s to 2.44s wall. With the ASAN lane environment (BUN_DESTRUCT_VM_ON_EXIT=1,detect_leaks=1): before 9.53s with both children exiting 134, after the list entry that environment is not applied. Also passes withUSE_SYSTEM_BUN=1.Background
--bailmakesbun teststop after the first failing test. bun test should produce reporter outfile when --bail is specified #26851 was about the JUnit outfile not being written in that case.test/no-validate-leaksan.txtlists test files for whichscripts/runner.node.mjsdoes not setBUN_DESTRUCT_VM_ON_EXITanddetect_leaks=1on the ASAN lane.test.concurrentto this file as part of a broad speed pass. This PR is scoped to the one file and also strengthens the assertions.Notes
The first CI run (build 110365) failed on the x64-asan lane with exit code 134 on both tests and 8.7s per test. That is the LeakSanitizer abort described above. The fix for the bail exit itself is in #39010 and out of scope here.
The stderr checks use
toContainrather than a snapshot of the whole stream. Open PRs #39010, #39276, #38974 and #38265 change parts of the bail and reporter output, and a whole-stream snapshot would conflict with them for no gain. The XML snapshot is the thing under test.The failure body in the XML contains
at fail.test.ts:2:40. The fixture sources are built from fixed strings, not indented template literals, so the column is stable.test/js/junit-reporter/junit.test.jssnapshots the same shape on every platform.[auto-merge] gate passed · iteration 1 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 1
evidence per changed file
root cause · written by the author bot
The regression test for issue #26851 was slow because each of its five tests spawned a separate
bun testchild one after another, and under ASAN every child paid several seconds of startup, while the assertions only checked that the JUnit outfile existed. The fix runs the independent cases concurrently through a shared runner that captures stdout, stderr, the exit code and the report once per fixture set, then asserts the full normalized JUnit document shape, the bail message, the summary line and the exact exit code. A follow-up strips the CI environment variables that make the reporter …